IGNITE-28426 Fix bug preventing SqlDdlGenerator from converting value classes with nested pojos annotated with SqlQueryField#7912
Conversation
… classes with nested pojos annotated with SqlQueryField ** Added tests
There was a problem hiding this comment.
Pull request overview
Fixes an issue in the migration tools’ SQL DDL generation where value classes containing nested POJOs annotated with @QuerySqlField were not handled correctly during QueryEntity enrichment, preventing expected table definition generation.
Changes:
- Add a regression test and a new test model covering a value class with an annotated nested POJO.
- Adjust
SqlDdlGenerator.populateQueryEntityto treat non-natively-supportedQueryEntityfield types as POJO-mapped and move them toward “extra fields” handling. - Expose
TypeInspector.isPrimitiveType(with Javadoc) for cross-package reuse.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| migration-tools/modules/migration-tools-commons/src/test/java/org/apache/ignite/migrationtools/sql/sql/SqlDdlGeneratorTest.java | Adds a parameterized regression test for nested annotated POJO handling. |
| migration-tools/modules/migration-tools-commons/src/main/java/org/apache/ignite/migrationtools/types/TypeInspector.java | Makes isPrimitiveType public for use in SQL DDL generation logic. |
| migration-tools/modules/migration-tools-commons/src/main/java/org/apache/ignite/migrationtools/sql/SqlDdlGenerator.java | Updates POJO detection and field handling logic during QueryEntity population. |
| migration-tools/modules/migration-tools-commons-tests/src/main/java/org/apache/ignite/migrationtools/tests/models/NestedPojoWithAnnotations.java | Introduces a test model with @QuerySqlField on a primitive and a nested POJO field. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // We also want to remove fields non-compliant with AI3 so that they are handled in the extra fields. | ||
| for (var e : qe.getFields().entrySet()) { | ||
| String fieldType = ClassnameUtils.ensureWrapper(e.getValue()); | ||
|
|
||
| boolean isNativelySupportedType; | ||
| try { | ||
| Class<?> type = ClassUtils.getClass(this.clientClassLoader, fieldType); | ||
| isNativelySupportedType = TypeInspector.isPrimitiveType(type); | ||
| } catch (ClassNotFoundException ignored) { | ||
| // All natively supported types should be in the classpath. | ||
| // Some enums might still slip throught. | ||
| isNativelySupportedType = false; | ||
| } | ||
|
|
||
| // Remove from the field list, we will need to map it in the extra fields. | ||
| if (!isNativelySupportedType) { | ||
| mapsPojo = true; | ||
| qe.getFields().remove(e.getKey()); | ||
| qe.getKeyFields().remove(e.getKey()); | ||
| } |
There was a problem hiding this comment.
Modifying qe.getFields() while iterating qe.getFields().entrySet() (calling qe.getFields().remove(...) inside the enhanced for-loop) will throw ConcurrentModificationException for typical Map implementations (e.g., HashMap/LinkedHashMap). Collect keys to remove first (or use an explicit iterator and iterator.remove()), then update keyFields accordingly.
| isNativelySupportedType = TypeInspector.isPrimitiveType(type); | ||
| } catch (ClassNotFoundException ignored) { | ||
| // All natively supported types should be in the classpath. | ||
| // Some enums might still slip throught. |
There was a problem hiding this comment.
Typo in comment: "slip throught" should be "slip through".
| // Some enums might still slip throught. | |
| // Some enums might still slip through. |
| /** | ||
| * Whether the provided class, or it's derivatives, is natively supported. | ||
| * | ||
| * @param type Type. | ||
| * @return True or false. | ||
| */ |
There was a problem hiding this comment.
Grammar in Javadoc: "it's derivatives" should be "its derivatives" (possessive, not contraction).
https://issues.apache.org/jira/browse/IGNITE-28426
What was done: